Skip to content

trait_sel: sizedness + auto trait goals prefer alias candidates #144064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Jul 17, 2025

Fixes #143992

  • abd07de: Reverts trait_sel: MetaSized always holds temporarily #144016 so that MetaSized bounds are checked properly, and updates all the tests accordingly, including making tests/ui/sized-hierarchy/incomplete-inference-issue-143992.rs fail when it shouldn't
  • 90e61db: Prefer alias candidates over parameter environment candidates for sizedness, auto and default traits. tests/ui/sized-hierarchy/incomplete-inference-issue-143992.rs passes again, but tests/ui/generic-associated-types/issue-93262.rs starts failing when it shouldn't
  • e412062: No longer require that predicates of aliases hold in well-formedness checking of the alias. tests/ui/generic-associated-types/issue-93262.rs passes again

Each commit updates all the tests to their new output so it should be easy enough to see what the impact of each change individually is. After all of the changes, tests that pass when they didn't before or vice versa:

This had a crater run in #142712 (comment) alongside some other changes.

r? @lcnr
cc #142712 (this extracts part of that change)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jul 17, 2025
@rustbot

This comment has been minimized.

@davidtwco davidtwco added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Jul 17, 2025
@lcnr lcnr added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jul 17, 2025
@lcnr
Copy link
Contributor

lcnr commented Jul 17, 2025

wrt 6aa3732 it's not that we stop to normalize the predicates of projections, it's that when comfirming alias-bound/Projection candidates, we required that the alias is well-formed. While we need to normalize these obligations before proving them, the normalize isn't the relevant part, the "we need to prove them at all" is.

wrt tests/ui/higher-ranked/trait-bounds/normalize-under-binder/norm-before-method-resolution-opaque-type.rs see https://rust-lang.zulipchat.com/#narrow/channel/144729-t-types/topic/sizedness.20bounds.20in.20explicit_implied_predicates_of.20.28.23142712.29/near/526987384. It's intended that this test changes to be ambiguous.

tests/ui/generic-associated-types/issue-92096.rs fails to prove C::Connecting<'placeholder>: Send which is required when proving that the generator is Send. This is 'just' an instance of #110338

Did you already crater run this behavior?

@davidtwco
Copy link
Member Author

Did you already crater run this behavior?

We did a crater run in #142712 (comment), which had all of these changes and included implying MetaSized on associated types.

@davidtwco davidtwco force-pushed the prefer-alias-over-env-for-sizedness branch from 6aa3732 to e412062 Compare July 17, 2025 13:02
@davidtwco
Copy link
Member Author

Updated the README and corrected the descriptions of everything per your comment

davidtwco added a commit to davidtwco/rust that referenced this pull request Jul 17, 2025
@bors

This comment was marked as resolved.

@davidtwco davidtwco force-pushed the prefer-alias-over-env-for-sizedness branch from e412062 to fa46ce0 Compare July 18, 2025 12:08
@davidtwco
Copy link
Member Author

Rebased following #143545

@davidtwco davidtwco added the I-types-nominated Nominated for discussion during a types team meeting. label Jul 18, 2025
@davidtwco
Copy link
Member Author

davidtwco added a commit to davidtwco/rust that referenced this pull request Jul 18, 2025
@lcnr lcnr changed the title trait_sel: sizedness goals prefer alias candidates trait_sel: sizedness + auto trait goals prefer alias candidates Jul 18, 2025
@bors

This comment was marked as resolved.

For sizedness, default and auto trait predicates, now prefer non-param
candidates if any exist. As these traits do not have generic parameters,
it never makes sense to prefer an non-alias candidate, as there can
never be a more permissive candidate.
No longer require that we prove that the predicates of aliases hold when
checking the well-formedness of the alias. This permits more uses of GATs
and changes the output of yet more tests.
@davidtwco davidtwco force-pushed the prefer-alias-over-env-for-sizedness branch from fa46ce0 to f1e23b5 Compare July 21, 2025 12:36
davidtwco added a commit to davidtwco/rust that referenced this pull request Jul 21, 2025
@lcnr lcnr removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 28, 2025
@lcnr
Copy link
Contributor

lcnr commented Jul 28, 2025

This PR does two relevant changes we need to FCP. We've already tried the first change before in #92191 and then reverted the PR as this caused a regression. The second change is necessary to avoid this regression and is also generally desirable.

Sized and auto trait goals now prefer alias-bound candidates over where-bounds

See #132325 for our existing candidate preference rules. We currently prefer where-bounds over alias-bounds for all candidates. This preference (like all preference) is arbitrary to some degree.

We need to do this for the following example:

trait Bound<'a> {}
trait Trait<'a> {
    type Assoc: Bound<'a>;
}

fn impls_bound<'b, T: Bound<'b>>() {}
fn foo<'a, 'b, 'c, T>()
where
    T: Trait<'a>,
    for<'hr> T::Assoc: Bound<'hr>,
{
    impls_bound::<'b, T::Assoc>();
    impls_bound::<'c, T::Assoc>();
}

We need to do so when normalizing:

trait Iterator {
    type Item;
}
trait IntoIterator {
    type Item;
    type IntoIter: Iterator<Item = Self::Item>;
}

fn normalize<I: Iterator<Item = ()>>() {}
fn foo<I>()
where
    I: IntoIterator,
    I::IntoIter: Iterator<Item = ()>,
{
    // normalizing `<I::IntoIter as Iterator>::Item` has two candidates:
    // - alias-bound normalizing to `<I as IntoIterator>::Item`
    // - where-bound normalizing to `()`
    normalize::<I::IntoIter>();
}

However, arbitrary candidate preference is a mess, so this is sometimes incorrect, either if the where-bound actually talks about a different alias, or different trait arguments:

trait Bound<'a> {}
trait Trait<'a> {
    type Assoc: Bound<'a>;
}

fn impls_bound<'b, T: Bound<'b>>() {}
fn foo<'a, 'b, T>()
where
    T: for<'hr> Trait<'hr>,
    <T as Trait<'b>>::Assoc: Bound<'a>,
{
    // Using the where-bound for `<T as Trait<'a>>::Assoc: Bound<'a>`
    // unnecessarily equates `<T as Trait<'a>>::Assoc` with the
    // `<T as Trait<'b>>::Assoc` from the env.
    impls_bound::<'a, <T as Trait<'a>>::Assoc>();
    // For a `<T as Trait<'b>>::Assoc: Bound<'b>` the self type of the
    // where-bound matches, but the arguments of the trait bound don't.
    impls_bound::<'b, <T as Trait<'b>>::Assoc>();
}

While figuring out whether where-bounds or alias-bounds have correct trait arguments isn't possible in general, we do know that alias-bound candidates always apply to exactly the right self type - the alias itself.

This means that in cases where the trait doesn't have any non-self parameters, prefering alias-bound candidates over where-bounds is always correct.

There is one edge case here, since #120752 we may also get alias-bound candidates from a nested self-type, so prefering alias-bound over where-bound candidates can be incorrect:

trait OtherTrait<'a> {
    type Assoc: ?Sized;
}

trait Trait
where
    <Self::Assoc as OtherTrait<'static>>::Assoc: Sized,
{
    type Assoc: for<'a> OtherTrait<'a>;
}

fn impls_sized<T: Sized>() {}
fn foo<'a, T>()
where
    T: Trait,
    for<'hr> <T::Assoc as OtherTrait<'hr>>::Assoc: Sized
{
    impls_sized::<<T::Assoc as OtherTrait<'a>>::Assoc>();
}

To avoid this issue, I propose limiting the preference of alias-bound over where-bound candidates only to the alias self-bounds, not indirect ones.

It also feels weird to me to implicitly change candidate preference depending on whether your trait has any generic arguments, so I propose to only change the preference rules for traits which are already special and guaranteed to not have any non-self arguments: Sized and auto traits.

To sum it up: we change our candidate preference rules to prefer alias self-bound candidates over where-bounds for Sized and auto traits. We continue to prefer where-bounds over alias bounds from nested self types and for all other traits.

Stop proving alias where-bounds when using alias-bounds

We currently prove the where-bounds of aliases when using an alias-bound candidate. This is generally redundant. We should have already proven these when checking whether the alias is well-formed.

However, we may have only ever proven well-formedness while the alias was still higher-ranked. If we then prove their where-bounds after instantiating this binder with placeholders, we get spurious errors due to missing implied bounds.

trait Bound {}
impl<'a, T> Bound for &'a T {}
trait Trait {
    type Assoc<'a>: Bound
    where
        Self: 'a;
}
impl<T> Trait for T {
    type Assoc<'a>
        = &'a T
    where
        Self: 'a;
}

// Using a higher ranked where-bound to prove itself succeeds,
// we don't check does not check whether it's WF after we've
// instantiated it. However, as normalizing an alias also requires
// proving its where-bounds, this function can only be called
// with types which are `'static`.
fn prove_hr<T: Trait>()
where
    for<'a> T::Assoc<'a>: Bound,
{
    prove_hr::<T>(); // ok
}
fn via_alias_bound<T: Trait>() {
    prove_hr::<T>(); // err, requires `T: 'static`, will compile with this PR
}
// Normalizing an associated item always checks its where-bounds.
fn via_impl_err<T>() {
    prove_hr::<T>(); // err, requires `T: 'static`
}

Ignoring the implied bounds of binders is generally unsound, e.g. #25860 and #84591.

The question is whether ignoring these implied bounds when using alias-bound candidates introduces any additional exploitable unsoundnesses:

  • This only enables us to prove a where-bound whose self type is a higher-ranked alias
  • To actually use this higher-ranked where-bound we need to instantiate the alias, at which point we should end up proving WF. As trait and alias parameters are all invariant, we can't hide erase the implied bound as in Implied bounds on nested references + variance = soundness hole #25860.
  • I cannot think of an alternative way to use this alias-bound in a bad way. I've spent a few hours playing with this and wasn't able to come up with an exploit

cc @steffahn would love for you to also take a look at this if you're interested. We already have the behavior of this PR with -Znext-solver.

I am not totally confident that it's impossible to exploit this in theory, even though it feels unlikely to me. However, I do feel quite confident that any such exploit will be highly artificial and will get fixed once we properly support implied bounds for the existing unsoundnesses without requiring any additional effort or consideration.

I personally think that checking WF of the alias when using its implied bounds feels somewhat arbitrary and am in favor of removing this check.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 28, 2025

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-types-nominated Nominated for discussion during a types team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

where bounds regression in beta+nightly
5 participants